Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: determine connected components #34

Merged
merged 6 commits into from
Apr 22, 2024

Conversation

SilasPeters
Copy link
Contributor

@SilasPeters SilasPeters commented Apr 14, 2024

Description

This commit allows determining the connected components, and visualises them in the ConnectedComponents scene by coloring the tiles.

In the process of doing so, I fixed some orientation bugs in the Tile classes, notably the Corner and TSection.

Testability

See for yourself, and good around! Look for the ConnectedComponents scene.

Checklist

  • Set the proper pull request name
  • Merged main into your branch
  • Added a scene to the game for your changes

@SilasPeters SilasPeters requested a review from Tboefijn April 14, 2024 17:07
@SilasPeters SilasPeters self-assigned this Apr 14, 2024
@SilasPeters SilasPeters marked this pull request as draft April 14, 2024 17:18
@SilasPeters SilasPeters force-pushed the feat/0078-spawn-teleporters---connected-components branch from b921321 to 14b0707 Compare April 14, 2024 17:33
@SilasPeters SilasPeters marked this pull request as ready for review April 14, 2024 17:33
Copy link
Contributor

@joachimdekker joachimdekker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I left a few comments. You have also made a lot of changes to Thijs' code. Please request a review from him.

aplib.net-demo/Assets/Scripts/Tiles/Corner.cs Outdated Show resolved Hide resolved
aplib.net-demo/Assets/Scripts/WFC/Grid.cs Show resolved Hide resolved
aplib.net-demo/Assets/Scripts/WFC/Grid.cs Outdated Show resolved Hide resolved
aplib.net-demo/Assets/Scripts/WFC/Grid.cs Outdated Show resolved Hide resolved
Copy link
Contributor

@Tboefijn Tboefijn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did a quick review because I don't have a lot of time. I left some comments but didn't review the code in depth.

aplib.net-demo/Assets/Scripts/Tiles/Corner.cs Outdated Show resolved Hide resolved
aplib.net-demo/Assets/Scripts/WFC/Grid.cs Outdated Show resolved Hide resolved
aplib.net-demo/Assets/Scripts/WFC/Grid.cs Outdated Show resolved Hide resolved
aplib.net-demo/Assets/Scripts/WFC/Grid.cs Outdated Show resolved Hide resolved
@SilasPeters SilasPeters force-pushed the feat/0078-spawn-teleporters---connected-components branch 2 times, most recently from 59ac5aa to 995efeb Compare April 15, 2024 18:19
Copy link
Contributor

@joachimdekker joachimdekker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good overal. Had a few nitpicks.

aplib.net-demo/Assets/Scripts/WFC/Cell.cs Outdated Show resolved Hide resolved
aplib.net-demo/Assets/Scripts/WFC/Grid.cs Outdated Show resolved Hide resolved
joachimdekker
joachimdekker previously approved these changes Apr 22, 2024
aplib.net-demo/Assets/Scripts/Tiles/Straight.cs Outdated Show resolved Hide resolved
aplib.net-demo/Assets/Scripts/Tiles/Direction.cs Outdated Show resolved Hide resolved
aplib.net-demo/Assets/Scripts/Tiles/Direction.cs Outdated Show resolved Hide resolved
aplib.net-demo/Assets/Scripts/Tiles/Direction.cs Outdated Show resolved Hide resolved
aplib.net-demo/Assets/Scripts/WFC/Grid.cs Outdated Show resolved Hide resolved
aplib.net-demo/Assets/Scripts/WFC/Grid.cs Outdated Show resolved Hide resolved
Comment on lines 171 to 175
if (cell.Tile.CanConnectInDirection(East) && neighbour.X > cell.X && neighbour.Tile.CanConnectInDirection(West))
connectedNeighbours.Add(neighbour);
else if (cell.Tile.CanConnectInDirection(West) && neighbour.X < cell.X && neighbour.Tile.CanConnectInDirection(East))
connectedNeighbours.Add(neighbour);
else if (cell.Tile.CanConnectInDirection(North) && neighbour.Y > cell.Y && neighbour.Tile.CanConnectInDirection(South))
connectedNeighbours.Add(neighbour);
else if (cell.Tile.CanConnectInDirection(South) && neighbour.Y < cell.Y && neighbour.Tile.CanConnectInDirection(North))
connectedNeighbours.Add(neighbour);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't really understand this if statement. They all result in the same code. It seems like this should be a big or statement, hopefully formatted somewhat nicely.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're damn right.

aplib.net-demo/Assets/Scripts/WFC/Cell.cs Outdated Show resolved Hide resolved
aplib.net-demo/Assets/Scripts/Tiles/TSection.cs Outdated Show resolved Hide resolved
aplib.net-demo/Assets/Scripts/WFC/GridPlacer.cs Outdated Show resolved Hide resolved
Tboefijn
Tboefijn previously approved these changes Apr 22, 2024
@SilasPeters SilasPeters force-pushed the feat/0078-spawn-teleporters---connected-components branch from 8f14356 to 8c40cfe Compare April 22, 2024 09:57
This commit allows determining the connected components, and visualises them in the ConnectedComponents scene by coloring the tiles
This prevents primitive blindness. Now code can more easily be read
@SilasPeters SilasPeters force-pushed the feat/0078-spawn-teleporters---connected-components branch from d59a145 to c728115 Compare April 22, 2024 10:10
Copy link
Contributor

@Tboefijn Tboefijn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@SilasPeters SilasPeters merged commit 12ec0d9 into main Apr 22, 2024
2 checks passed
@SilasPeters SilasPeters deleted the feat/0078-spawn-teleporters---connected-components branch April 22, 2024 10:17
Copy link

🎉 This PR is included in version 3.1.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants